sound/midi: midi_poll is always failing. #1887
Conversation
|
Thank you for taking the time to contribute to FreeBSD!
Please review CONTRIBUTING.md, then update and push your branch again. |
|
@christosmarg could you please take a look at this? The patch seems obviously right. The commit log message needs to be fixed up a little. |
|
I think the fix is right. Ref: the very first version of the midi.c from NetBSD [1]. [1] NetBSD/src@48bae9e#diff-dadfe9a149c74b946edbe5f3764799be7ba664df592dcc04bef1d70f003a2a10 |
sys/dev/sound/midi/midi.c
Outdated
| mtx_unlock(&m->qlock); | ||
|
|
||
| return (revents); | ||
| return (revents ? 1 : 0); |
There was a problem hiding this comment.
Don't we need to return revents as it is? Why are you returning a bool?
There was a problem hiding this comment.
I must check that. The generic poll system call returns the count of file descriptors having matching events.
There was a problem hiding this comment.
This is what's returned to usersland by poll(2), but the kernel poll() methods return event bitmasks.
|
The change is good. Can you please combine those into one commit and rebase on top |
sys/dev/sound/midi/midi.c
Outdated
| events |= events & (POLLOUT | POLLWRNORM); | ||
| revents |= events & (POLLOUT | POLLWRNORM); | ||
|
|
||
| if (revents == 0) { |
There was a problem hiding this comment.
I think so (for now).
There was a problem hiding this comment.
I'm talking about the revents == 0. Before, because of the bug, this would always be true, but now it won't.
There was a problem hiding this comment.
You're right; I think this block should be split to handle the case when both POLLIN and POLLOUT are passed. Just updated the code.
There was a problem hiding this comment.
Note: I missed this problem because I focused on the locks.. I can tell you that I removed 'qlock' from midi.c entirely and I'm currently testing it (on 13.5, as I cannot manage to compile main).
| } | ||
|
|
||
| mtx_unlock(&m->lock); | ||
| mtx_unlock(&m->qlock); |
There was a problem hiding this comment.
Not related to the patch, but I'm pretty sure there is a lock order reversal here. Have you tested this code path? If there is, I can propose a patch. Let me know.
There was a problem hiding this comment.
I just swapped these unlocks, seems strange to me. Anyway let restore the right order.
There was a problem hiding this comment.
You can open a new PR for this issue.
|
Committed: https://cgit.freebsd.org/src/commit/?id=8f8b8e4af91d4e158caf6ba4b728482311bfc7c3 Closing the PR. |
Sponsored by: The FreeBSD Foundation MFC after: 1 week Reviewed by: christos Pull Request: #1887
Due to two typos, midi_poll was always failing, as the 'revents' field was not properly set. Signed-off-by: Nicolas Provost <[email protected]>
4 participants
Due to two typos, midi_poll was always failing, as the 'revents' field was not properly set.
Signed-off-by: Nicolas Provost [email protected]